-
Notifications
You must be signed in to change notification settings - Fork 17.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Battery critical/low actions #7213
Battery critical/low actions #7213
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Michael. I think this stuff adds important functionality that benefits people doing long range flights.
I do see a couple of PRs that potentially conflict with this one. getting this one in will probably simplify those other PRs. This needs a rebase though.
I think we now have vehicle specific docs, so some of this can be moved to the library, and all vehicles benefit from it :)
What is terminate action ???
AP_Float _curr_amp_per_volt; /// voltage on current pin multiplied by this to calculate current in amps | ||
AP_Float _curr_amp_offset; /// offset voltage that is subtracted from current pin before conversion to amps | ||
AP_Int32 _pack_capacity; /// battery pack capacity less reserve in mAh | ||
AP_Int16 _watt_max; /// max battery power allowed. Reduce max throttle to reduce current to satisfy t his limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespaces in comment
#7432 provides a way to customize what happens when a FS gets triggered. So these two can get combined ? |
ef087d8
to
637621c
Compare
6634324
to
753b523
Compare
float voltage_resting_estimate; // voltage with sag removed based on current and resistance estimate | ||
float resistance; // resistance calculated by comparing resting voltage vs in flight voltage | ||
bool healthy; // battery monitor is communicating correctly | ||
cells cell_voltages; // battery cell voltages in millivolts, 10 cells matches the MAVLink spec |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the added value of indentation surpass the value of breaking git blame ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been caught! :)
@amilcarlucas I'd hold off on reviewing to much yet, been pushing more for linking to others :) Moving a large pile of additional stuff into AP_BattMonitor first. |
bbd8e97
to
1c4b3dc
Compare
This has been pushed with support for all vehicles (Tracker is a no op at the moment, but the skeleton is there for others). Summary of changes:
Outstanding Issues:
Needed Testing (SITL is fine):
If the vehicle maintainers can chime in at this point that would be great! |
f3bb10b
to
8673019
Compare
Pushed the nitpick fixes already. Your list looks correct to me. To be fair I was actually expecting some of the motor current limiting stuff to work already on quadplane since I could see the parameters, but I'm glad I wasn't banking on it. |
checking that the param conversion is working ! |
@khancyr Great, thanks! After chatting with tridge the main risk with battery compensation was that quadplanes could use the forward battery rather then the vertical battery, which wouldn't help, and the other concern is if you have a single battery during transition. I've pushed an additional commit that allows AP_Motors to specify what battery index is used (which is primarily useful for multi battery setups), and the default value won't change behavior for anyone. The other change there is that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems OK to me. I tested compassmot (as requested by @WickedShell) and it seems to produce similar values to what master produces.
@rmackay9 the main concern was if you could trigger a battery failsafe that actually caused the vehicle to try and do something during the compassmot procedure. |
@WickedShell, I tested attempting to trigger a radio failsafe during the compassmot procedure but wasn't able to. In particular I did this:
|
@rmackay9 right, that's not the same thing unfortunately. The old code used to explicitly disable the battery failsafes by changing the parameter values. That was removed in this version, however if we need to reintroduce a disable for the cal that's fine, but the modification of parameters was a bad way to do it. |
One small thing is the default BATT_LOW_VOLT is too high. It shows up as "11" on my IRIS anyway but the default previously was lower (10.4 maybe?). By the way, i also tried triggering a battery failsafe during compass mot but wasn't able to. |
Also removes the example script, as it was broken, and causing more headaches then it was worth
ff626dc
to
34e4d94
Compare
// @Param: BAT_IDX | ||
// @DisplayName: Battery compensation index | ||
// @Description: Which battery monitor should be used for doing compensation | ||
// @Range: 0 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be values instead of range? If you want to go with range, then add the @Increment: 1
.
34e4d94
to
c523768
Compare
c523768
to
a30868a
Compare
Merged, thanks! |
This builds upon #7177 to enable different failsafe levels per connected battery, as well as allowing the vehicle to define a critical and a low battery action. This also serves as a replacement for #5000.
This is a WIP at the moment as this has only touched plane so far. Given the scope of the changes, and how it touches all the vehicles I'd like to get feedback on the proposed approach before I look at reworking copter/rover/sub to all support this as well. (I don't really know what the best actions are for a lot of those vehicles, although I think that some like terminate can be supported on all vehicles if AFS is enabled. (The initial pr would probably just present the options to have the current behavior on those vehicles unless someone wants to steer further).
A couple of points worth noting here at a top leve that I would appreciate feedback onl:
BATT_LOW_TIMER
parameter). I can't see a strong reason why you would want a different timeout for critical vs long, and would prefer to rename this asBATT_FS_TIMER
but it would be yet another param rename. Having 2 timers is easy, but I'm not sure its worth the param space.